Add opt-in HTTP retry support (REST + PDP)#120
Conversation
- Add built-in retry support with exponential backoff for transient failures - Retry on network errors and status codes: 408, 429, 500, 502, 503, 504 - Respect Retry-After headers for rate limiting (429) - Default: 3 retries with exponential backoff (1s, 2s, 4s) - Add IRetryConfig interface for full configuration control - Support separate retry config for PDP calls via pdpRetry option - Enable POST retry for PDP calls (check operations are idempotent) - Add comprehensive unit tests (33 tests) - Update README with retry configuration documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sort imports alphabetically to satisfy eslint sort-imports rule. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds first-class HTTP retry support to the SDK (REST API + PDP/OPA clients), including exponential backoff, optional Retry-After handling, and user-configurable retry behavior via retry / pdpRetry options.
Changes:
- Introduces retry utilities (
resolveRetryConfig, backoff/jitter delay calculation,Retry-Afterparsing) and an Axios response interceptor to perform retries. - Wires retry configuration into
Permit(API client) andEnforcer(PDP/OPA clients, with POST retries enabled for PDP/OPA). - Adds unit tests for retry utilities and documents configuration in the README.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Lockfile adjustments from dependency resolution changes. |
| src/utils/retry.ts | Retry configuration/types, default condition, Retry-After parsing, and delay calculation utilities. |
| src/utils/retry-interceptor.ts | Axios response interceptor implementing retry logic + logging. |
| src/index.ts | Exposes retry types/constants and installs retry interceptor for REST API calls. |
| src/enforcement/enforcer.ts | Installs retry interceptor for PDP/OPA clients and enables POST retries for idempotent checks. |
| src/config.ts | Adds retry and pdpRetry options to SDK configuration. |
| src/tests/unit/retry.spec.ts | Adds AVA unit tests for retry utilities and basic SDK export/config checks. |
| README.md | Documents retry configuration and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // For PDP calls, enable POST retry since check operations are idempotent | ||
| const pdpRetryWithPost = { | ||
| ...pdpRetryConfig, | ||
| retryMethods: [...pdpRetryConfig.retryMethods, 'POST'], |
There was a problem hiding this comment.
pdpRetryWithPost appends 'POST' to retryMethods without deduplicating. If the user already includes POST, this can create duplicates. Consider ensuring uniqueness (e.g., via new Set(...)) when constructing the PDP method list.
| // For PDP calls, enable POST retry since check operations are idempotent | |
| const pdpRetryWithPost = { | |
| ...pdpRetryConfig, | |
| retryMethods: [...pdpRetryConfig.retryMethods, 'POST'], | |
| // For PDP calls, ensure POST is retried (check operations are idempotent), without duplicates | |
| const pdpRetryWithPost = { | |
| ...pdpRetryConfig, | |
| retryMethods: [...new Set([...pdpRetryConfig.retryMethods, 'POST'])], |
There was a problem hiding this comment.
Done — the PDP method list is now built with [...new Set([...pdpRetryConfig.retryMethods, 'POST'])], so POST can't be duplicated even if the user already includes it. Methods are also normalized to uppercase in resolveRetryConfig now, so the de-dup is case-correct. Fixed in 12f1a98.
zeevmoney
left a comment
There was a problem hiding this comment.
Thanks for building this out — retry support is genuinely useful. A few things before merge, most important first (a duplicate-write risk). Note: Copilot already flagged the missing interceptor tests, the parseRetryAfter partial-number parse, and a retryMethods de-dup nit, so those aren't repeated here.
- Give the PDP enforcer its own dedicated axios instance so PDP-only POST retries never land on the shared REST client (no double-writes). - Default retries to opt-in: DEFAULT_RETRY_CONFIG.enabled=false; passing a retry config object opts in. Avoids silent latency change on upgrade. - parseRetryAfter rejects non-digit delta-seconds (e.g. "5abc"). - resolveRetryConfig returns a fresh copy and normalizes retryMethods to uppercase; freeze DEFAULT_RETRY_CONFIG. - Dedup POST in the PDP retryMethods via Set. - Use a real Symbol for the retry-count key. - Add test:unit script so unit specs run in CI; add interceptor tests and deterministic Retry-After HTTP-date tests. - README: document the opt-in default and PDP-vs-REST POST distinction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
All four points are addressed in
Copilot's nits in the same commit: interceptor tests, digits-only |
- Revert the retry-count key from Symbol back to the string '__permitRetryCount'. axios's mergeConfig (run on every request() during a retry) copies only string-keyed props via Object.keys, so a Symbol key was dropped each retry — the counter reset to 0 and caused an infinite retry loop. Comment explains why a string is required. - Add isolation regression tests: REST and PDP use separate axios instances, and the REST instance does not retry POST while the PDP instance does (caught the Symbol infinite loop). - config.ts: correct the retry JSDoc to describe opt-in behavior; note that a custom axiosInstance applies to REST only (PDP/OPA use dedicated internal instances); fix the opaAxiosInstance JSDoc. - README: note that a custom axiosInstance applies to REST only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use the battle-tested axios-retry library for the retry mechanics (counting, re-dispatch, timeout reset, abort handling) instead of the custom response interceptor that previously shipped an infinite-loop bug. The public config surface (retry/pdpRetry, IRetryConfig) and our delay/condition policy (resolveRetryConfig, calculateRetryDelay, parseRetryAfter) are unchanged — retry-interceptor.ts is now a thin adapter translating the resolved config into axiosRetry() options: - retries = maxRetries; retryCondition = per-instance method filter + our network/status condition (REST excludes POST, PDP includes it); retryDelay reuses calculateRetryDelay (1-based retryCount -> 0-based attempt); shouldResetTimeout = true; onRetry preserves the log line. - Add axios-retry ^4.5.0 (1 transitive dep, is-retry-allowed). - Rewrite the interceptor tests as behavior tests through real axios instances + a rejecting adapter (isolation, POST distinction, retry count, non-retryable status, disabled path, retryCount mapping). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Close the coverage gap from the axios-retry migration review: - a retryable GET that fails once then succeeds resolves (proves a retry actually recovers, not just exhausts); - a 429 with `retry-after: 2` schedules a 2000ms delay, proving the Retry-After header flows through calculateRetryDelay via the axios-retry retryDelay callback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Delete NON_RETRYABLE_STATUS_CODES: it was defined and publicly re-exported but never used in any retry logic (the condition uses RETRYABLE_STATUS_CODES). Drop its definition, the index.ts re-export, and the tests that only asserted its value. - Make DEFAULT_RETRY_METHODS module-private; it's only used in-module by DEFAULT_RETRY_CONFIG and imported nowhere. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- index.ts: strip POST from the REST retryMethods regardless of user config, so the REST client never retries non-idempotent writes (hard guarantee, symmetric with the enforcer adding POST for PDP/OPA). - retry.ts: clarify that maxRetries is the number of retries after the initial request (default 3 = up to 4 attempts); freeze the DEFAULT_RETRY_METHODS array so the exported DEFAULT_RETRY_CONFIG can't be mutated by consumers (Object.freeze was shallow). - retry-interceptor.ts: reword the retry log to "retry N/M" to avoid implying total attempts. - README: clarify "3 retries (up to 4 total attempts)". - tests: mark the two Date.now()-stubbing HTTP-date tests serial to avoid concurrent races; add tests for the frozen default and for REST refusing to retry POST even when the user adds it to retryMethods. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Freeze RETRYABLE_STATUS_CODES (readonly number[]) so the exported array can't be mutated to change SDK-wide retry behavior. - Skip installing the REST retry interceptor when the POST-filtered retryMethods is empty (e.g. user passes retryMethods: ['POST']), avoiding a no-op interceptor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zeevmoney
left a comment
There was a problem hiding this comment.
Approved: CI green, mergeable, no blocking reviews from other reviewers. Squash-merging.
Summary
Adds opt-in HTTP retry support to the SDK for transient failures, built on the
axios-retrylibrary. Retries are off by default — existing callers see no behavior change unless they pass aretryconfig.retry(orpdpRetry) config object is provided;retry: falsekeeps them off.maxDelay; honorsRetry-After(delay-seconds or HTTP-date) on429.408, 429, 500, 502, 503, 504.retry(REST API) andpdpRetry(PDP/OPA, falls back toretry).POST(check operations are idempotent); the REST API never retriesPOST.What kind of change does this PR introduce?
Feature (with tests and docs). Linear: PER-13890.
Current behavior
The SDK issues HTTP requests to the Permit REST API and to the PDP with no automatic retry. Any transient failure — a network error, a
429rate-limit, or a5xx— surfaces to the caller immediately on the first attempt.New behavior
Opt-in retry is wired onto the SDK's HTTP clients via
axios-retry, driven by the SDK's own delay/condition policy.retry/pdpRetryare unset by default, so retries are off and existing callers are unaffected. Providing aretryconfig object turns them on (3 attempts, exponential backoff by default);retry: falseorenabled: falsekeeps them off.src/config.ts) — two optional fields onIPermitConfig:retry?: IRetryConfig | false— REST API client.pdpRetry?: IRetryConfig | false— PDP/OPA clients; falls back toretrywhen omitted.src/utils/retry.ts) —resolveRetryConfig,calculateRetryDelay(exponential backoff + jitter,maxDelaycap,Retry-After),parseRetryAfter, andRETRYABLE_STATUS_CODES. The retry condition is "network error or retryable status," filtered by the configured methods (normalized to uppercase).src/utils/retry-interceptor.ts) — a thin adapter that configuresaxios-retryon an instance from the resolved policy (retries,retryCondition,retryDelay,shouldResetTimeout,onRetrylogging).axios-retryowns retry counting, re-dispatch, per-attempt timeout reset, and request-abort handling.src/index.tsinstalls retry on the REST API instance;src/enforcement/enforcer.tsinstalls it on dedicated PDP and OPA instances withPOSTadded to the retried methods.POST(idempotent checks); the REST API does not retryPOST, so non-idempotent writes are never repeated.axiosInstanceapplies to the REST API only; PDP and OPA use dedicated internal axios instances.src/index.ts):IRetryConfig,RetryConditionFn,RETRYABLE_STATUS_CODES. All additions are optional/additive — existing config and call sites are unchanged.Files changed
src/utils/retry.ts,src/utils/retry-interceptor.ts— new (config/delay policy +axios-retryadapter)src/config.ts—retry/pdpRetryconfig fieldssrc/index.ts— install REST retry + re-exportssrc/enforcement/enforcer.ts— dedicated PDP/OPA instances + retry (POSTenabled for PDP/OPA)src/tests/unit/retry.spec.ts,src/tests/unit/retry-interceptor.spec.ts— unit + behavior testsREADME.md— retry configuration documentationpackage.json/yarn.lock— addaxios-retryTest summary
resolveRetryConfigopt-in semantics,calculateRetryDelay,parseRetryAfter).POSTwhile PDP does, retry count, non-retryable status, disallowed method, disabled path,Retry-Afterend-to-end, and success-after-retry.test:unitscript (run-s test:*).